Skip to content

Allow messenger handler to return non-resource output#2878

Closed
karser wants to merge 1 commit into
api-platform:2.4from
karser:non-uri-output
Closed

Allow messenger handler to return non-resource output#2878
karser wants to merge 1 commit into
api-platform:2.4from
karser:non-uri-output

Conversation

@karser

@karser karser commented Jun 25, 2019

Copy link
Copy Markdown
Contributor
Q A
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets #2860 #2810 #2803
License MIT

Hi folks!

Let's imagine ResetPasswordRequest / ResetPasswordRequestHandler case from the api-platform docs (https://api-platform.com/docs/core/messenger/).

Before api-platform v2.4.3 it was possible to return the output dto object (ResetPasswordResponse) directly from the messenger handler. It was very convenient, and that's what dto's are for - return a custom response without exposing an entity.
As of v2.4.3 it started to complain that ResetPasswordResponse must be a resource, so this PR allows returning the non-resource output.

@teohhanhui

teohhanhui commented Jun 26, 2019

Copy link
Copy Markdown
Contributor

I've already explained to you on Slack why this is wrong. But let me say it again: it's the job of the DataTransformer to transform a resource to the output DTO. If you want to return a non-resource, just remove the output option and it should work.

@teohhanhui teohhanhui closed this Jun 26, 2019
@karser

karser commented Jun 26, 2019

Copy link
Copy Markdown
Contributor Author

Sorry for bothering you again with this case but I'm not seeing an alternative in the approach you suggest. Moreover, it was working in 2.4.2. Why a bugfix update breaks the backwards compatibility?

If you want to return a non-resource, just remove the output option and it should work.

the output model is auto-documented. If I remove the output, the documentation will be missing.

it's the job of the DataTransformer to transform a resource to the output DTO

the code would be too scattered if ResetPasswordRequestHandler returned a resource and ResetPasswordRequestDataTransformer returned an output object. Also, the resource doesn't have to be convertable to the output dto.

My points are:

  • returning the output was described in the first version of dto docs added by @lyrixx. The messenger+output is supposed to be a successor of that approach.
  • output is amazingly useful - it solves the problem of custom methods and adds the output to swagger doc

I'm wondering what is @dunglas and @soyuka opinion on this case.

@soyuka

soyuka commented Jun 26, 2019

Copy link
Copy Markdown
Member

I've already explained to you on Slack why this is wrong. But let me say it again: it's the job of the DataTransformer to transform a resource to the output DTO. If you want to return a non-resource, just remove the output option and it should work.

Wait we'ŕe in the context of a MessageHandler, isn't it a bit different? Could the MessageHandler act as a data transformer? Shouldn't we support non-resources anyway (to me we definitely should)?

the code would be too scattered if ResetPasswordRequestHandler returned a resource and ResetPasswordRequestDataTransformer returned an output object.

To me this makes sense and removes some useless boilerplate code.

I don't really see anything wrong with the PR here, it just simplifies this use case no?

@karser

karser commented Jul 1, 2019 via email

Copy link
Copy Markdown
Contributor Author

@teohhanhui

teohhanhui commented Jul 2, 2019

Copy link
Copy Markdown
Contributor

@soyuka @karser

If you want to return a non-resource, just remove the output option and it should work.

the output model is auto-documented. If I remove the output, the documentation will be missing.

This is the actual issue, a more general problem. We should allow documenting any classes.

Otherwise it's already correct. Just don't use output class if you don't want to use a DataTransformer. It's unnecessary.

@karser

karser commented Jul 2, 2019

Copy link
Copy Markdown
Contributor Author

@teohhanhui You are right, the documentation - that's what I use the output for.

We should allow documenting any classes

How do you imagine that? A special option or using swagger_context?

Tweaking swagger_context won't work, looks like the DTO class has to be added to swagger definitions.

App\UseCase\ForgotPasswordRequest:
    collectionOperations:
        post:
            path: '/users/forgot-password-request'
            messenger: true
#            output: 'App\UseCase\ForgotPasswordResponse'
            status: 200
            swagger_context:
                responses:
                    200:
                        description: 'Response'
                        schema:
                            type: 'App\UseCase\ForgotPasswordResponse'
#                            $ref doesn't seem to be working as well

image

@teohhanhui

teohhanhui commented Jul 4, 2019

Copy link
Copy Markdown
Contributor

Let's continue that discussion in #2147

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants